Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: tini as ENTRYPOINT and related cosmetics #12707

Merged
merged 10 commits into from
Mar 10, 2023

Conversation

gczuczy
Copy link
Contributor

@gczuczy gczuczy commented Mar 3, 2023

Note on DCO:

If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

@gczuczy gczuczy changed the title tini as ENTRYPOINT and related cosmetics feat: tini as ENTRYPOINT and related cosmetics Mar 3, 2023
@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 3, 2023

Entrypoint related kubernetes docs:
https://kubernetes.io/docs/tasks/inject-data-application/define-command-argument-container/
https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#container-v1-core

Point is, once the ENTRYPOINT is properly set in the image, no wrappers are needed to start the service through an init (tini in our case), which takes are of signal propagation and other stuff.

entrypoint.sh is still retained. If someone happens to use the latest image, but still has old manifests, then removing it would break those system. To avoid breaking systems with the entrypoint.sh removal, I would suggest waiting a complete minor release's cycle, then remove it - this would date it for 2.8.

Also, the interpreter has been changed to /bin/sh, because that's available in busybox already if someone wishes to use a near-distroless image, which as little fluff as possible. The entrypoint.sh does not use an bash extensions, and has been tested in our systems internally for the past couple of months.

This effectively removes the dependency on /bin/sh, which is another step for making a distroless image possible: #9029

@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 3, 2023

Reminder for the helm chart: argoproj/argo-helm#1883

@gczuczy gczuczy marked this pull request as ready for review March 3, 2023 07:31
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.02 🎉

Comparison is base (b8fdfb5) 47.77% compared to head (9c6670a) 47.79%.

❗ Current head 9c6670a differs from pull request most recent head b50820d. Consider uploading reports for the commit b50820d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #12707      +/-   ##
==========================================
+ Coverage   47.77%   47.79%   +0.02%     
==========================================
  Files         246      246              
  Lines       41985    41968      -17     
==========================================
+ Hits        20057    20058       +1     
+ Misses      19929    19910      -19     
- Partials     1999     2000       +1     

see 5 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@tuananh
Copy link
Contributor

tuananh commented Mar 3, 2023

I wrote a proposal here #12708

thought it might be related

@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 3, 2023

I wrote a proposal here #12708

thought it might be related

ArgoCD doesn't need much to run an absolutely distroless image.

The following binaries are needed besides argocd, if you disable gpg support:

  1. sh
  2. cp
  3. tar
  4. rm
  5. tini
  6. git
  7. git-remote-https
  8. helm
  9. kustomize

Helm and kustomize can obviousyl be built statically, so no deps on an distro there.

rm is already eliminated by a merged PR, so it won't be needed by 2.7
cp is currently being used for putting the cmp-server under /var/run in an initcontainer. Once PR12508 is merged, it has a function for copying files without cp, and that should be able to replace the dependency on cp.

IIRC sh is only needed for entrypoint, aside from plugins - but plugins are in a sidecar now, so that's partially a different topic.

tar still has to be investigated.

Git is interesting, @crenshaw-dev mentioned that the git cli calls could be replaced with - iirc - gitea, so the git binary shouldn't be needed once that's done. And up until that point git can be built statically with a bit of effort - so no deps on any distros (including git-remote-https).

After this, literally there's no distro needed, just having the argocd binaries and the above mentioned statically linked binaries in the container. You can already build an image like this, based FROM scratch, and it does work. And probably even gpg can be supplied in a statically linked fashion.

@tuananh
Copy link
Contributor

tuananh commented Mar 3, 2023

glad to see there's initiative working on this. looking forward to 2.7 release.

@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 3, 2023

@ishitasequeira Could you please review this one?

@crenshaw-dev
Copy link
Member

Once #12508 is merged, it has a function for copying files without cp, and that should be able to replace the dependency on cp.

Would you be willing to make the cp part its own PR? That would let us get it in for 2.7 even if we can get the full PR done by then.

@crenshaw-dev
Copy link
Member

if you disable gpg support

I think we'd need to ship both a -distroless and an Ubuntu-based image, if we can't get gpg working on the distroless image.

Git is interesting, @crenshaw-dev mentioned that the git cli calls could be replaced with - iirc - gitea, so the git binary shouldn't be needed once that's done. And up until that point git can be built statically with a bit of effort - so no deps on any distros (including git-remote-https).

I think the git binary is pretty thoroughly enmeshed in our code right now. Moving to a library would be quite a bit of work. I think a statically-linked binary may be the easier short-term approach.

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gczuczy would you mind adding a note to entrypoint.sh and to the upgrade notes that the container specs are changing, and that in 2.8 people must update the manifests instead of just updating the image tag?

@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 7, 2023

@gczuczy would you mind adding a note to entrypoint.sh and to the upgrade notes that the container specs are changing, and that in 2.8 people must update the manifests instead of just updating the image tag?

Done:

  • Added note to 2.6-2.7 notes on it, so people can see to it preemptively
  • Added the 2.7-2.8.md without adding it to indexes, with a note on this.
  • Added tghe 2.6-2.7 notes to indexes, so it shows up properly on the webui

@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 7, 2023

if you disable gpg support

I think we'd need to ship both a -distroless and an Ubuntu-based image, if we can't get gpg working on the distroless image.

I don't know about gpg, as I haven't even tried, because we are not using it at all. It both mght or might not possible to easily provide a static binary for it, I'm unable to say, as I haven't checked. The git build system needed minimal patching for the static builds to work, might be the same for gpg as well. I have no idea. Later I might be able to check on it, unfortunately my hands are absolutely full right now.

But providing two kind of images, or at least a Dockerfile for a minimal (near "distroless") image sounds like a good idea. People who need that are typically corporate guys who have a significant focus on security, and they can be expected to be able to customize/alter an image to their specific needs.

@gczuczy
Copy link
Contributor Author

gczuczy commented Mar 7, 2023

Once #12508 is merged, it has a function for copying files without cp, and that should be able to replace the dependency on cp.

Would you be willing to make the cp part its own PR? That would let us get it in for 2.7 even if we can get the full PR done by then.

There you go: #12751

@crenshaw-dev
Copy link
Member

Thanks @gczuczy! lgtm. I'm going to bring this up at the contributors' meeting to make sure everyone is aware of and likes the approach.

@jannfis
Copy link
Member

jannfis commented Mar 10, 2023

But providing two kind of images, or at least a Dockerfile for a minimal (near "distroless") image sounds like a good idea.

I'm not sure whether that wouldn't create kind of a significant maintenance overhead here, as we'd have to support two distinct images.

I'm not against distroless, but imho, maintaining two docker images (or Dockerfiles) would not be the right thing to do.

@crenshaw-dev crenshaw-dev merged commit c2e0026 into argoproj:master Mar 10, 2023
yyzxw pushed a commit to yyzxw/argo-cd that referenced this pull request Aug 9, 2023
* Use tini as the ENTRYPOINT implicitly

Signed-off-by: Gergely Czuczy <[email protected]>

* Explicitly call /bin/cp instead of relying on PATH

Signed-off-by: Gergely Czuczy <[email protected]>

* POSIX sh is sufficient for entrypoint.sh

Signed-off-by: Gergely Czuczy <[email protected]>

* Add 2.6-2.7 to docs indexes

Signed-off-by: Gergely Czuczy <[email protected]>

* Add note on tini on entry to 2.6-2.7 upgrade notes

Signed-off-by: Gergely Czuczy <[email protected]>

* Added note to 2.7-2.8 notes on tini as entry

Signed-off-by: Gergely Czuczy <[email protected]>

---------

Signed-off-by: Gergely Czuczy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants